-
Notifications
You must be signed in to change notification settings - Fork 709
update ProgramPrepare to accept wasm instead of statedb and code #4284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Igor Braga <[email protected]>
| debugInt = 1 | ||
| } | ||
|
|
||
| wasm, err := getWasm(statedb, programAddress, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could use getWasmFromContractCode which is called by getWasm. My concern comes from this comment:
nitro/arbos/programs/native.go
Lines 279 to 280 in db29e55
| // addressForLogging may be empty or may not correspond to the code, so we need to be careful to use the code passed in separately | |
| wasm, err := getWasmFromContractCode(statedb, code, params, false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we used getWasmFromContractCode make sure the isActivation flag is set to true, because handle is only used in ethCall? so it is simulating contract activation?, other than that for the potential edgecase you are presenting, I trust your judgement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is exactly why you want to use getWasmFromContractCode and not getWasm. There are weird edgeCases like delegateCall and reading code from programAddressForLogging might not be correct, but the code you have here is correct and can be passed to getWasmFromCode
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4284 +/- ##
===========================================
- Coverage 57.12% 32.82% -24.31%
===========================================
Files 476 476
Lines 56785 56785
===========================================
- Hits 32441 18638 -13803
- Misses 19506 34877 +15371
+ Partials 4838 3270 -1568 |
❌ 6 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
KolbyML
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
| debugInt = 1 | ||
| } | ||
|
|
||
| wasm, err := getWasm(statedb, programAddress, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we used getWasmFromContractCode make sure the isActivation flag is set to true, because handle is only used in ethCall? so it is simulating contract activation?, other than that for the potential edgecase you are presenting, I trust your judgement
| moduleHashPtr unsafe.Pointer, | ||
| addressForLoggingPtr unsafe.Pointer, | ||
| codePtr unsafe.Pointer, | ||
| codeSize uint64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need wasmSize instea of codeSize, and right after wasmPtr
| debugInt = 1 | ||
| } | ||
|
|
||
| wasm, err := getWasm(statedb, programAddress, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is exactly why you want to use getWasmFromContractCode and not getWasm. There are weird edgeCases like delegateCall and reading code from programAddressForLogging might not be correct, but the code you have here is correct and can be passed to getWasmFromCode
Updates
ProgramPrepareto acceptwasminstead ofstatedbandcode.Closes NIT-4407